-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make DataFrame API consuming (#4621) #4624
Conversation
@@ -265,20 +265,18 @@ impl SessionContext { | |||
if_not_exists, | |||
or_replace, | |||
}) => { | |||
let input = Arc::try_unwrap(input).unwrap_or_else(|e| e.as_ref().clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of unfortunate, I hope to rework this all as part of #4617
@@ -431,7 +431,7 @@ mod tests { | |||
) | |||
.await?; | |||
|
|||
ctx.register_table("t1", ctx.table("test")?)?; | |||
ctx.register_table("t1", Arc::new(ctx.table("test")?))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is still problematic as it leads to a reference cycle (#2659), I intend to address this as part of reworking these interfaces
FYI @andygrove I'm not sure if this has some implication for the python bindings? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
That's a good point .. thanks, I'll take a look (cc @isidentical) |
I think there a couple of places where we need to make some adjustments (and another interesting idea that comes with this is whether we want tests with nightly datafusion revisions on the datafusion-python) but (at least by a very light look) I don't think this should be a problem. |
Thank you for taking a look 👍 |
ca93d5b
to
23f5e40
Compare
I'm going to merge this in as I don't think it all that controversial and there are changes backed up on it, happy to address feedback in follow up PRs |
Benchmark runs are scheduled for baseline = 3611d91 and contender = 5c558e9. 5c558e9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4621
Part of #4349
Relates to #4617
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?